-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update bindings #72
Update bindings #72
Conversation
Codecov Report
@@ Coverage Diff @@
## development #72 +/- ##
===============================================
+ Coverage 21.07% 21.22% +0.15%
===============================================
Files 17 16 -1
Lines 3194 3176 -18
===============================================
+ Hits 673 674 +1
+ Misses 2521 2502 -19
|
535931d
to
05e3708
Compare
ThresholdEncryptionError = _ferveo.ThresholdEncryptionError | ||
InvalidShareNumberParameter = _ferveo.InvalidShareNumberParameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, looking forward to using DKG size 3 of 5 on testnets!
class Keypair: | ||
@staticmethod | ||
def random() -> Keypair: | ||
... | ||
|
||
@staticmethod | ||
def from_secure_randomness(data: bytes) -> Keypair: | ||
def from_secure_randomness(bytes: bytes) -> Keypair: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name shadowing of the bytes
function here strikes me as a bit unusual, even in the context of a stub. I'd suggest sticking with some variant of data
(payload
, secure_bytes
) or at minimum prepend with an underscore (_bytes
.)
Do the parameter stubs need to be identical to the actual binding definition? If so, then this will lead to the bytes
function also being overloaded within the scope of this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I got weirded out a little myself, but wasn't sure if I should comment on it :)
@@ -42,10 +40,11 @@ class SecretKeyFactory: | |||
... | |||
|
|||
@staticmethod | |||
def from_secure_randomness(data: bytes) -> SecretKeyFactory: | |||
def from_secure_randomness(seed: bytes) -> SecretKeyFactory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you decided to use a different name here compared to the ferveo key above - seed
is a better name for this param considering it's intended usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to use secure_randomness
in both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, Just a few comments about name shadowing.
290263a
to
bb1c65e
Compare
bb1c65e
to
0043f80
Compare
0043f80
to
7f3a7f2
Compare
policy_encrypting_key: PublicKey, | ||
plaintext: bytes, | ||
conditions: Optional[Conditions] | ||
self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this spacing correct / necessary (eg. linter)? It really increases the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my IDE's formatter, sorry about that.
Now that I think about it I should adopt formatting style & utilities from nucypher
into Python bindings everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good - no need to be sorry. I guess we do need to think about linting in these other repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an expert on Rust, but it looks good! 👍
Just a few of comments.
nucypher-core-wasm/Cargo.toml
Outdated
umbral-pre = { version = "0.10.0", features = ["bindings-wasm"], git = "https://github.com/piotr-roslaniec/rust-umbral.git", rev = "fd2e16f9304b9dab85c9a8947a61d962ff136131" } | ||
ferveo = { package = "ferveo-pre-release", features = ["bindings-wasm"], git = "https://github.com/nucypher/ferveo.git", rev = "cea467e0bd48a096f70dd1c7ca24a7e4bd88b3d4" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO: here to note that these are not final versions? (or at least I guess so)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is not getting merged until those downstream dependencies get released. In the future, I will document this in a PR description or using a TODO.
... | ||
|
||
|
||
def encrypt(message: bytes, aad: bytes, dkg_public_key: DkgPublicKey) -> Ciphertext: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm. Is this aad
variable correctly spelled? what does it stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It stands for Additional Authenticated Data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes sense. Thanks
nucypher-core/src/dkg.rs
Outdated
type SessionSecretFactorySeedSize = U32; | ||
// the size of the seed material for key derivation | ||
type SessionSecretFactoryDerivedKeySize = U32; | ||
// the size of the derived key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO the comments should be placed above the line and not under the line they refer. (this seems like a formatting tool thing)
type SessionSecretFactorySeedSize = U32; | |
// the size of the seed material for key derivation | |
type SessionSecretFactoryDerivedKeySize = U32; | |
// the size of the derived key | |
// the size of the seed material for key derivation | |
type SessionSecretFactorySeedSize = U32; | |
// the size of the derived key | |
type SessionSecretFactoryDerivedKeySize = U32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo fmt
is messing these up constantly for me, I may need to edit formatting rules.
nucypher-core/Cargo.toml
Outdated
@@ -10,8 +10,8 @@ readme = "README.md" | |||
categories = ["cryptography", "no-std"] | |||
|
|||
[dependencies] | |||
umbral-pre = { version = "0.10.0", features = ["serde"] } | |||
ferveo = { package = "ferveo-pre-release", version = "0.2.0" } | |||
umbral-pre = { version = "0.10.0", features = ["serde"], git = "https://github.com/piotr-roslaniec/rust-umbral.git", rev = "fd2e16f9304b9dab85c9a8947a61d962ff136131" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO: here to note that these are not final versions? (or at least I guess so)
95651ff
to
b3dbb5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ✌️
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
umbral-pre-python
typings #70ferveo
Python typings #71[email protected]
update #69 (comment)Why it's needed:
Notes for reviewers:
[email protected]
update #69